fungibles methods do not consume AssetId
#4224
Conversation
| assert_eq!(Fungibles::name(&asset_id.clone().into()), Vec::from(expected_name),); | ||
| assert_eq!(Fungibles::symbol(&asset_id.clone().into()), Vec::from(expected_symbol),); |
There was a problem hiding this comment.
Possible to remove the .clones in scenarios like this or?
There was a problem hiding this comment.
All these tests helper functions provide asset_id as impl Into<Fungibles::AssetId> + Clone. I would need to change the API of these functions, then a lot of test callsites.
I was thinking it's not worth the effort and the noise since these are only tests.
| fn total_issuance(asset: Self::AssetId) -> Self::Balance { | ||
| match Criterion::convert(asset) { | ||
| fn total_issuance(asset: &Self::AssetId) -> Self::Balance { | ||
| match Criterion::convert(asset.clone()) { |
There was a problem hiding this comment.
Hmm I wonder if Criterion::convert should also take a ref?
There was a problem hiding this comment.
I initially tried, but got lost in the sea of errors. There are so many generic types and trait bounds used here, hard to follow.
I will give it another try.
There was a problem hiding this comment.
Can't make it work.. got as far as dianpopa@090c74e, maybe someone can help or do it in another PR.
|
Context: This is good clean up. It's because |
|
PR has a lot of changes, but the vast majority is test code because of the changed inner functions APIs. The "main" API changes are small I think. |
|
@liamaharon @gilescope is there anything else required here? |
| @@ -723,7 +723,10 @@ fn reserve_transfer_native_asset_from_system_para_to_para() { | |||
| let sender_balance_before = test.sender.balance; | |||
| let receiver_assets_before = PenpalA::execute_with(|| { | |||
| type ForeignAssets = <PenpalA as PenpalAPallet>::ForeignAssets; | |||
| <ForeignAssets as Inspect<_>>::balance(system_para_native_asset_location.clone(), &receiver) | |||
| <ForeignAssets as Inspect<_>>::balance( | |||
| &system_para_native_asset_location.clone(), | |||
There was a problem hiding this comment.
Does this and all similar still need .clone()?
|
The CI pipeline was cancelled due to failure one of the required jobs. |
|
@liamaharon is this fix for #3968 still wanted? |
|
I agree this is a good fix and a tedious one (the amount of compiler errors, number of files), we should push for it to get merged before more conflicts pile up. @liamaharon thoughts? |
Fungiblesmethods now takeAssetIdby reference instead of value where the value is not needed.pallet-assetsmethods now also takeAssetIdby reference instead of value where the value is not needed.This avoids a lot of cloning across the codebase.
There are more objects and traits that could do the same, but this PR already blew up in size as I was changing the most relevant ones. This "small" change is actually pretty big..
fixes #3968
Polkadot address: 14rY421S6kMMSRo9GkbMrWiVjD2UnbhFp6Boy9pDeYnBy1hD